Conversation
Athena's SQL DROP SCHEMA ... CASCADE fails when the schema contains Iceberg tables. Add an athena__edr_drop_schema dispatch that drops every relation individually first (the adapter handles Iceberg vs Hive differences in its drop_relation implementation) before removing the now-empty schema. Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
👋 @devin-ai-integration[bot] |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds two dbt macros — Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI workflow (GitHub)
participant DBT as dbt operation
participant Adapter as DB adapter
participant Athena as Athena / Metastore
CI->>DBT: run operation elementary_tests.drop_test_schemas
DBT->>Adapter: call athena__edr_drop_schema(database, schema)
Adapter->>Athena: list_relations_without_caching(database, schema)
Athena-->>Adapter: return relations
loop for each relation
Adapter->>Athena: drop_relation(relation)
Athena-->>Adapter: relation dropped
end
DBT->>DBT: dbt.drop_schema(database, schema)
DBT-->>CI: operation complete
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Add a drop_test_schemas macro that drops all schemas created by a CI test run (base + xdist worker schemas _gw0 through _gw7). Add an 'if: always()' step to test-warehouse.yml that calls this macro, ensuring schemas are cleaned up even when pytest is cancelled or crashes before its own teardown runs. Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
devin-ai-integration
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test-warehouse.yml:
- Around line 206-213: The "Drop test schemas" job step uses the plain dbt
binary but should follow the earlier conditional that uses ~/.local/bin/dbt when
inputs.dbt-version == 'fusion'; update the run command that calls "dbt
run-operation elementary_tests.drop_test_schemas" to conditionally invoke
"~/.local/bin/dbt" when inputs.dbt-version == 'fusion' (matching the earlier
steps) so the same dbt binary is used for fusion builds and avoid invoking the
wrong dbt installation.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/test-warehouse.ymlintegration_tests/dbt_project/macros/clear_env.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- integration_tests/dbt_project/macros/clear_env.sql
Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
integration_tests/dbt_project/macros/clear_env.sql (1)
75-77: Individual relation drop failures will halt the cleanup loop.If
adapter.drop_relation(relation)fails for any relation (e.g., permissions, transient error), the remaining relations won't be dropped. While the workflow usescontinue-on-error, partial cleanup within this macro could leave orphaned relations.Consider wrapping individual drops to continue on failure:
♻️ Suggested approach (if dbt/Jinja supports it)
{% for relation in relations %} - {% do adapter.drop_relation(relation) %} + {% do log("Dropping relation: " ~ relation, info=true) %} + {# Note: dbt macros don't have native try/catch; consider adapter.execute with error handling if available #} + {% do adapter.drop_relation(relation) %} {% endfor %}If robust error handling is needed, this may require a Python-based approach or accepting the current behavior with workflow-level
continue-on-error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration_tests/dbt_project/macros/clear_env.sql` around lines 75 - 77, The loop over relations currently calls adapter.drop_relation(relation) directly so a single failure aborts the macro; modify the macro to handle per-relation failures by wrapping each adapter.drop_relation(relation) call in a try/except-style guard (or Jinja-equivalent error handler) that catches the error, logs or records the failed relation, and continues the loop so remaining relations are attempted; if Jinja/dbt templating cannot catch exceptions reliably, move this logic into a Python macro or helper that iterates relations and calls adapter.drop_relation inside a try/except to ensure individual drop failures do not stop the cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@integration_tests/dbt_project/macros/clear_env.sql`:
- Around line 75-77: The loop over relations currently calls
adapter.drop_relation(relation) directly so a single failure aborts the macro;
modify the macro to handle per-relation failures by wrapping each
adapter.drop_relation(relation) call in a try/except-style guard (or
Jinja-equivalent error handler) that catches the error, logs or records the
failed relation, and continues the loop so remaining relations are attempted; if
Jinja/dbt templating cannot catch exceptions reliably, move this logic into a
Python macro or helper that iterates relations and calls adapter.drop_relation
inside a try/except to ensure individual drop failures do not stop the cleanup.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/test-warehouse.ymlintegration_tests/dbt_project/macros/clear_env.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test-warehouse.yml
…'t persist) Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
1 participant
fix: add Athena-specific edr_drop_schema and always-run schema cleanup step
Summary
The nightly cleanup job fails on Athena because
DROP SCHEMA … CASCADE(issued by the defaultedr_drop_schema) errors when the schema contains Iceberg tables:This adds an
athena__edr_drop_schemadispatch that works around the issue by:adapter.list_relations_without_caching()adapter.drop_relation()(the dbt-athena adapter already handles Iceberg vs Hive table differences internally — it uses the Glue API path by default)Additionally, stale schemas were accumulating across all adapters because schemas are only cleaned up during pytest's session teardown (
--clear-on-end), which doesn't run when jobs are cancelled (cancel-in-progress: true) or crash. This PR also adds:drop_test_schemasmacro that drops all schemas a CI run may have created (base schema + xdist worker schemas_gw0through_gw7, each with its_elementarycounterpart)if: always()step intest-warehouse.ymlthat calls this macro — only for cloud targets (snowflake,bigquery,redshift,databricks_catalog,athena), since local adapters (postgres, clickhouse, trino, dremio, spark, duckdb) don't persist schemas between runs. This matches the pattern already used in the elementary repo's test workflow.Updates since last revision
dbtbinary path in the new "Drop test schemas" workflow step to use~/.local/bin/dbtwheninputs.dbt-version == 'fusion', matching the conditional used by other steps (e.g., "Install dependencies", "Check DWH connection").snowflake,bigquery,redshift,databricks_catalog,athena), per reviewer feedback that local adapters don't persist schemas.Review & Testing Checklist for Human
drop_test_schemas: The macro constructs elementary schema names as<base_schema>_elementary<suffix>. Confirm this matches whatgenerate_schema_name("elementary", node)+ the xdist suffix actually produces. If the naming convention differs, schemas will be silently skipped (no error, just no-op).adapter.drop_relation()works in run-operation context: In dbt-athena,athena__drop_relationreadsconfig.get('native_drop', default=false). In arun-operationcontext there's no model config, so it should default tofalseand use the Glue API path (drop_relation_glue). Confirm this is the correct/working path for Iceberg tables.contains(fromJSON('["snowflake","bigquery","redshift","databricks_catalog","athena"]'), ...). If new cloud targets are added totest-all-warehouses.ymlin the future, this list will need manual updating.adapter.drop_relation()fails on one table (e.g., Iceberg DROP timeout), the macro will abort the current schema's loop. The workflow step hascontinue-on-error: trueas a safety net, but remaining schemas in thedrop_test_schemasloop won't be attempted either.Drop test schemasstep works.Notes
if: always()cleanup step is best-effort — if the job is forcibly killed bycancel-in-progressbefore reaching this step, schemas will still leak. The nightly cleanup job remains the ultimate safety net.Summary by CodeRabbit
New Features
Bug Fixes
Documentation